Implement edges function for IGraph #26
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26 +/- ##
=========================================
- Coverage 6.47% 5.27% -1.20%
=========================================
Files 8 8
Lines 4342 4323 -19
=========================================
- Hits 281 228 -53
- Misses 4061 4095 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
This is pretty impressive for jumping into it without having had prior exposure to any of these tools, good job! I think there is a much more efficient way to set it up though, check out the link I shared. And we need to add tests.
| function Graphs.edges(g::IGraph) | ||
| edge_list = Vector{Graphs.edgetype(g)}() | ||
| adjlist = IGAdjList() | ||
| LibIGraph.adjlist_init(g,adjlist,LibIGraph.IGRAPH_ALL,LibIGraph.IGRAPH_LOOPS_TWICE,LibIGraph.IGRAPH_MULTIPLE) |
There was a problem hiding this comment.
Good find! This seems like a pretty expensive allocating operation. I would expect there to be a way to iterate over edges without having to create the adjlist.
I suspect this will be a better way: https://igraph.org/c/html/latest/igraph-Iterators.html#edge-iterators
There was a problem hiding this comment.
and we also need correctness tests
There was a problem hiding this comment.
what do you mean by correctness tests?
There was a problem hiding this comment.
A new feature usually comes with additions to the test suite, so that one can attest that the feature is correctly implemented. More importantly, such tests will be executed for any change in the future, ensuring that the feature is not accidentally broken.
For instance, here you are implementing an edges function. A convenient correctness test would be some self-consistency check. E.g. create a graph by specifically adding some random set of edges. Then call the edges function on that graph and verify that it returns the same set as the initial set you started with. This type of "round trip" consistency checks are a powerful way to detect issues.
There was a problem hiding this comment.
commenting on the edge iterators: you will probably also need this to convert edge id to pair of vertex ids https://igraph.org/c/html/latest/igraph-Basic.html#igraph_edge
There was a problem hiding this comment.
igraph_edge — Returns the head and tail vertices of an edge.
igraph_error_t igraph_edge(
const igraph_t *graph, igraph_int_t eid,
igraph_int_t *from, igraph_int_t *to
); for using this function, I have to pass in some pointer for from and to as I will be using a raw binding so how do i make a pointer in julia?
There was a problem hiding this comment.
This is a idiom in the C language. A C function can not return sophisticated objects like tuples, so instead you give the C function pointers to the locations of each object that you want to "return". So in C even outputs need to be provided as arguments to a function.
In the Julia wrapper for this library, some of these are made a bit nicer. Check the description in the library. There should be a julia function LibIGraph.igraph_edge that is a barebones wrapper of the C function and requires these pointer arguments, but there should also be an autogenerated nice function LibIGraph.edge that directly returns what would have been written in those pointers (and thus does not require pointer arguments). At least that should be the case if the automatic wrapper worked out for this function.
|
the failing tests, as you guessed in your email, are unrelated to your work -- these are just tests that have not been properly set yet |
2 participants
No description provided.